Skip to content

Use secrets module for secure random token generation#898

Closed
4gust wants to merge 2 commits intodevfrom
4gust/random-gen-improve
Closed

Use secrets module for secure random token generation#898
4gust wants to merge 2 commits intodevfrom
4gust/random-gen-improve

Conversation

@4gust
Copy link
Copy Markdown
Contributor

@4gust 4gust commented Apr 7, 2026

Replace random.sample() with secrets.choice() for generating PKCE code verifiers, OAuth2 state parameters, and OIDC nonces.

Why: random uses Mersenne Twister (not cryptographically secure). secrets uses os.urandom() (CSPRNG), preventing prediction-based attacks. Also fixes an entropy reduction from random.sample() drawing without replacement.

Risk: Low — secrets requires Python 3.6+ (already minimum). No public API changes

…eration

Replace random.sample() with secrets.choice() for generating PKCE code
verifiers, OAuth2 state parameters, and OIDC nonces. The random module
uses Mersenne Twister which is not cryptographically secure. The secrets
module uses os.urandom(), providing a CSPRNG suitable for security tokens.

This also fixes a subtle entropy reduction caused by random.sample()
drawing without replacement, which prevented character repetition.
Copilot AI review requested due to automatic review settings April 7, 2026 13:49
@4gust 4gust requested a review from a team as a code owner April 7, 2026 13:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the security of auth-code-flow protections in msal.oauth2cli by switching token/parameter generation from random.sample() (Mersenne Twister, non-CSPRNG) to secrets.choice() (CSPRNG), and also removes the “no replacement” sampling behavior that unnecessarily reduced the effective search space.

Changes:

  • Replace nonce generation in OIDC auth code flow with secrets.choice(...).
  • Replace PKCE code verifier generation with secrets.choice(...) over an RFC7636-compliant alphabet.
  • Replace OAuth2 state generation with secrets.choice(...).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
msal/oauth2cli/oidc.py Switch OIDC nonce generation to secrets for replay-attack mitigation strength.
msal/oauth2cli/oauth2.py Switch PKCE verifier and OAuth2 state generation to secrets, keeping RFC7636 character requirements.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@gladjohn
Copy link
Copy Markdown
Contributor

gladjohn commented Apr 7, 2026

CoPilot suggest adding this optional test, otherwise looks good

# tests/test_oauth2_security.py - New test file (suggested)

import unittest
from msal.oauth2cli import oauth2
import string

class TestTokenGenerationSecurity(unittest.TestCase):
    """Verify secure token generation using secrets module."""
    
    def test_pkce_verifier_format(self):
        """Verify PKCE verifier is valid and uses correct alphabet."""
        pkce = oauth2._generate_pkce_code_verifier(length=43)
        verifier = pkce["code_verifier"]
        
        # Check length
        self.assertEqual(len(verifier), 43)
        
        # Check alphabet compliance (RFC 7636)
        allowed_chars = set(string.ascii_letters + string.digits + "-._~")
        self.assertTrue(all(c in allowed_chars for c in verifier))
        
        # Verify code_challenge exists and is base64url encoded (no padding)
        self.assertIn("code_challenge", pkce)
        self.assertNotIn(b"=", pkce["code_challenge"])
    
    def test_pkce_verifier_custom_length(self):
        """Test PKCE verifier with valid custom lengths."""
        for length in [43, 64, 128]:
            pkce = oauth2._generate_pkce_code_verifier(length=length)
            self.assertEqual(len(pkce["code_verifier"]), length)
    
    def test_token_uniqueness(self):
        """Verify generated tokens are unique (randomness test)."""
        # Generate multiple tokens - should all be different
        verifiers = [oauth2._generate_pkce_code_verifier()["code_verifier"] 
                     for _ in range(10)]
        self.assertEqual(len(set(verifiers)), 10, "Generated tokens should be unique")
        
        # Also test state generation
        from msal.oauth2cli.oauth2 import Client
        from unittest.mock import Mock
        
        client = Client(
            server_configuration={"authorization_endpoint": "https://example.com/auth"},
            client_id="test-client",
            http_client=Mock()
        )
        
        states = [client.initiate_auth_code_flow(scope=["test"])["state"] 
                  for _ in range(10)]
        self.assertEqual(len(set(states)), 10, "Generated states should be unique")

if __name__ == '__main__':
    unittest.main()

@4gust 4gust closed this Apr 7, 2026
@4gust 4gust deleted the 4gust/random-gen-improve branch April 7, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants